Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tasotarkistus data to rent model #773

Merged
merged 9 commits into from
Dec 5, 2024

Conversation

NC-jsAhonen
Copy link
Contributor

Add old_dwellings_in_housing_companies_price_index to rent model, to use tasotarkistus data on the Vuokrat tab.

@juho-kettunen-nc
Copy link
Contributor

I have questions about the model changes and what is needed for the initial UI changes, and what is needed in the future. Will discuss in a meeting with you.

@juho-kettunen-nc
Copy link
Contributor

We talked with Jukka. There will likely be more changes coming to this PR when the UI implementation becomes more refined, especially the saving functionality. Then it becomes more apparent which properties need to be explicitly saved in database, and which ones can be derived from existing properties.

For example, tasotarkistus type (20/20 or 20/10) must be saved along with the tasotarkistus, but maybe calculation dates can be derived from that type and the rent start date (e.g. 20 years from rent start date, then +10, then +10...).

There is a feature request for notifications for upcoming tasotarkistus dates for the rents that have it in near future, maybe 3 years/2 years/1 year in advance. That notification implementation might be slow to execute without dedicated columns for tasotarkistus dates, where DB indexes could be created. But the first notification would happen sometime in the decade 2040, so this is probably just a suggestion in code rather than the final implementation.

@NC-jsAhonen NC-jsAhonen force-pushed the MVJ-421-periodic-price-index-to-rent-types branch from 3359d3e to c13af49 Compare November 21, 2024 13:15
@NC-jsAhonen NC-jsAhonen force-pushed the MVJ-421-periodic-price-index-to-rent-types branch from 4467e63 to 4504ef8 Compare November 22, 2024 11:19
max_digits=8,
null=True,
)

Copy link
Contributor

@juho-kettunen-nc juho-kettunen-nc Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I think we need a 3rd new property: type of the tasotarkistus (20v/20v) or (20v/10v).

The Rent class is getting slightly populated, but it might still be OK because I just removed 4 fields about the seasonal rent.

Another possibility is to create a new model for PeriodicRentAdjustment which holds these 3 fields (index, adjustment type, and starting point figure). Then Rent would have a single ForeignKey property like periodic_rent_adjustment that can be null if the rent doesn't use the feature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you @henrinie-nc think?

@NC-jsAhonen NC-jsAhonen force-pushed the MVJ-421-periodic-price-index-to-rent-types branch from c77da90 to c06e0c2 Compare November 29, 2024 10:18
Copy link
Contributor

@juho-kettunen-nc juho-kettunen-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the discussed topics have been addressed, good work!

One topic stands, see the open comment about a adding a third new property to Rent class: type of the tasotarkistus (20v/20v) or (20v/10v).

Copy link
Contributor

@juho-kettunen-nc juho-kettunen-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This current state works for viewing. Editing view changes will come in a later PR

@NC-jsAhonen NC-jsAhonen merged commit 8e1639d into develop Dec 5, 2024
2 checks passed
@NC-jsAhonen NC-jsAhonen deleted the MVJ-421-periodic-price-index-to-rent-types branch December 5, 2024 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants